Skip to content

Conversation

@sarvekshayr
Copy link
Contributor

@sarvekshayr sarvekshayr commented Mar 27, 2024

What changes were proposed in this pull request?

Introduced a check on the size containers allowed to move during the balancing process.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10483

How was this patch tested?

Added a unit test balancerShouldMoveOnlyPositiveSizeContainers to assert that balancer selects only positive size containers.
Also tested using the existing test balancerShouldObeyMaxSizeLeavingSourceLimit() in TestContainerBalancerTask.java. It passed successfully, ensuring that the balancer selected only containers with a size greater than 0 bytes, even when zero-byte containers were present.

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarvekshayr Thanks for working over this, have minor comment

@kerneltime
Copy link
Contributor

Why is this change needed? If there is a skew in zero size objects it will show in the heartbeat load form a Datanode.

@sumitagrawl
Copy link
Contributor

sumitagrawl commented Mar 28, 2024

Why is this change needed? If there is a skew in zero size objects it will show in the heartbeat load form a Datanode.

@kerneltime
Its observed containerInfo is going -ve or "0" even there are blocks in it. This is observed and still root cause is not known. A workaround is handled to avoid delete container by using a flag earlier.
ContainerBalancer also have impact because of this and this PR will try to solve (till the actual problem is not found/resolved).

Copy link
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1

@siddhantsangwan siddhantsangwan self-requested a review April 2, 2024 05:44
Copy link
Contributor

@siddhantsangwan siddhantsangwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarvekshayr Thanks for working on this. The code change looks good, but we should have an explicit unit test that asserts balancer only selects positive size containers. This ensures we don't have to depend on another unit test that's meant to test a different behaviour.

@sarvekshayr
Copy link
Contributor Author

@sarvekshayr Thanks for working on this. The code change looks good, but we should have an explicit unit test that asserts balancer only selects positive size containers. This ensures we don't have to depend on another unit test that's meant to test a different behaviour.

Added a unit test to assert the expected behaviour of the balancer.

Copy link
Contributor

@siddhantsangwan siddhantsangwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, waiting for CI.

@kerneltime
Copy link
Contributor

@sarvekshayr can you look into the CI failures?

@siddhantsangwan
Copy link
Contributor

The balancer test had probably failed because of #6481. Recon failure looks unrelated.

@siddhantsangwan siddhantsangwan merged commit 80c2311 into apache:master Apr 10, 2024
@siddhantsangwan
Copy link
Contributor

Merged to master with green CI. Thanks @sarvekshayr and all the reviewers.

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Apr 17, 2024
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024
ptlrs pushed a commit to ptlrs/ozone that referenced this pull request Mar 8, 2025
…s with size greater than 0 bytes (apache#6447) (apache#112)

Co-authored-by: Sarveksha Yeshavantha Raju <79865743+sarvekshayr@users.noreply.github.com>
ivandika3 pushed a commit to ivandika3/ozone that referenced this pull request Oct 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants